Add fast paths for class/style string arguments in BlazeAttributeBag#168
Add fast paths for class/style string arguments in BlazeAttributeBag#168SanderMuller wants to merge 2 commits into
Conversation
|
/benchmark class |
|
/benchmark merge |
Benchmark Result: Class
Median of 10 attempts, 5000 iterations x 10 rounds, 31.77s total To run a specific benchmark, comment |
Benchmark Result: Merge
Median of 10 attempts, 5000 iterations x 10 rounds, 23.32s total To run a specific benchmark, comment |
Benchmark Result: Default
Median of 10 attempts, 5000 iterations x 10 rounds, 47.62s total To run a specific benchmark, comment |
|
Looks like this causes an attribute ordering issue in the FluxPro tests (you would need to dev-require Flux Pro with a valid license to run these): Tests\FluxProTest > chart: - <ui-chart wire:model="data" class="block [:where(&)]:relative w-full aspect-2/1" wire:ignore.children >
+ <ui-chart class="block [:where(&)]:relative w-full aspect-2/1" wire:model="data" wire:ignore.children >Alternatively feel free to add a separate comparison test for this |
When merge(), class(), or style() receive a simple string argument (the most common usage), skip the general-purpose loops and handle the merge directly. The shared class-merging logic is extracted into withMergedClass() to avoid duplication between merge() and class(). Uses array union to place the merged key first, matching the attribute ordering of the general merge() path's array_merge($defaults, $attrs). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
SanderMuller
force-pushed
the
autoresearch/attribute-bag-fast-paths
branch
from
22dea7d to
c7e51ea
Compare
Thanks. I do have a Flux Pro license, but figured it would be good to have a CI test so I took the option to add a test for it (and make the code pass it) |
The fast paths skipped merge()'s style normalization (trailing semicolon)
and appendable-first key ordering whenever the bag carried the other
appendable attribute, e.g.:
<x-merge-class wire:model="data" style="color:blue" />
Blade: class="..." style="color:blue;" wire:model="data"
Blaze: class="..." wire:model="data" style="color:blue"
Guard the class fast paths on the absence of a style attribute (and vice
versa for style()), and handle falsy current style values ('' / '0') the
way merge() does.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
calebporzio
left a comment
calebporzio
left a comment
There was a problem hiding this comment.
Really nice work here — the fast path is where the time goes, and it shows: I benchmarked locally and got class 3.56ms → 2.73ms (-23%) and merge 3.19ms → 2.77ms (-13%) blaze time vs main, right in line with the CI numbers.
While reviewing I wrote a batch of Blade-vs-Blaze comparison probes against the fast paths and found a few divergences beyond the ordering case @ganyicz caught — they all boil down to the same root cause: vanilla merge() also normalizes/reorders the other appendable attribute when it's in the bag, and the fast paths skipped that. For example:
<x-merge-class wire:model="data" style="color:blue" />
Blade: <div class="default-class" style="color:blue;" wire:model="data"></div>
Blaze: <div class="default-class" wire:model="data" style="color:blue"></div>(missing trailing ; on style + ordering), same story for ->class('...') with a style attribute present and ->style('...') with a class attribute present, plus a falsy-current-style edge (style="0" got dropped, style="" behaves differently than Blade).
Since the cheapest correct move is to just not take the fast path in those cases, I pushed 1e154c0 to your branch: guards the class fast paths on ! isset($this->attributes['style']) (and vice versa for style()), fixes the falsy-style handling, and adds comparison tests covering the combinations. All 14 of my probe cases now match Blade byte-for-byte, the full suite passes with this branch merged into current main (210 passed), and the benchmark win is unchanged (class 2.73ms, merge 2.78ms — the guards don't fire in the common case).
Feel free to rework my commit if you see a way to keep the fast path even when both appendables are present. Otherwise this looks good to me — thanks Sander!
3 participants
When
merge(),class(), orstyle()receive a simple string argument (the most common usage), skip the general-purpose loops and handle the merge directly.The shared class-merging logic is extracted into
withMergedClass()to avoid duplication betweenmerge()andclass().This replaces both PR #157 and #161 with a single combined PR.